Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FR-52] feat: Session usage monitor in react #2814

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

ironAiken2
Copy link
Contributor

@ironAiken2 ironAiken2 commented Nov 5, 2024

This PR resolves #2803 issue.

Changes:

  • Displays the resources allocated to the session. Currently, the compute session node does not include a live stat field, which shows the live stat information of the main kernel associated with the session.
  • Since each device has different usage, I've shown the usage below the progress bar to bring consistency to the design.

How to test:

  • Access the Detail page of the endpoint that is currently in service.
  • Verify that resource usage looks normal.
normal small screen
image.png image.png

Checklist: (if applicable)

  • Mention to the original issue
  • Documentation
  • Minium required manager version
  • Specific setting for review (eg., KB link, endpoint or how to setup)
  • Minimum requirements to check during review
  • Test case(s) to demonstrate the difference of before/after

Copy link

graphite-app bot commented Nov 5, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the size:L 100~500 LoC label Nov 5, 2024
Copy link
Contributor Author

ironAiken2 commented Nov 5, 2024


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • flow:merge-queue - adds this PR to the back of the merge queue
  • flow:hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

github-actions bot commented Nov 5, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
5.01% (-0.02% 🔻)
398/7948
🔴 Branches
4.32% (-0.06% 🔻)
237/5490
🔴 Functions
3.01% (-0.01% 🔻)
78/2595
🔴 Lines
4.93% (-0.02% 🔻)
383/7776
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴
... / SessionUsageMonitor.tsx
0% 0% 0% 0%

Test suite run success

124 tests passing in 14 suites.

Report generated by 🧪jest coverage report action from 0900b2e

@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from 3a48b36 to 8358664 Compare November 5, 2024 09:03
@ironAiken2 ironAiken2 marked this pull request as ready for review November 5, 2024 09:13
Copy link
Contributor

@agatha197 agatha197 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image.png
Memory doesn't have label, but CUDA memory has label.

react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from 8358664 to 63bd1b2 Compare November 12, 2024 09:15
@ironAiken2 ironAiken2 requested a review from agatha197 November 12, 2024 09:16
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from 63bd1b2 to e85e6a4 Compare November 13, 2024 03:17
@ironAiken2 ironAiken2 requested a review from agatha197 November 13, 2024 03:18
Copy link
Contributor

@agatha197 agatha197 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from e85e6a4 to 87e4942 Compare November 25, 2024 05:47
@ironAiken2 ironAiken2 requested a review from yomybaby November 25, 2024 05:51
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from 87e4942 to a1bc1c6 Compare November 25, 2024 09:18
Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found several things that need improvement and discussion. Let's discuss this PR tomorrow.

  • Should we include an icon?
  • Should the progress bar be larger?
  • How can we make it usable in the columns of the session list neo?
  • There are incorrect labels and units.

@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from a1bc1c6 to c15e3d9 Compare December 2, 2024 02:18
@ironAiken2 ironAiken2 marked this pull request as draft December 4, 2024 05:12
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from c15e3d9 to ca597df Compare December 4, 2024 07:33
@ironAiken2 ironAiken2 marked this pull request as ready for review December 4, 2024 07:39
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from ca597df to 3ef88fd Compare December 4, 2024 07:40
@ironAiken2 ironAiken2 requested a review from yomybaby December 4, 2024 07:41
Copy link
Contributor Author

The size property of progressProps or showIcon can be used to modify the visibility of the icon or the size of the progress bar.

@ironAiken2 ironAiken2 changed the title feat: Session usage monitor in react [FR-52] feat: Session usage monitor in react Dec 27, 2024
@ironAiken2 ironAiken2 marked this pull request as draft December 31, 2024 03:56
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch 4 times, most recently from 5f15ae8 to 22a1850 Compare January 2, 2025 02:17
@ironAiken2 ironAiken2 marked this pull request as ready for review January 2, 2025 02:17
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from 16426c7 to f84a448 Compare January 2, 2025 04:15
Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

graphite-app bot commented Jan 2, 2025

Merge activity

<!--
Please precisely, concisely, and concretely describe what this PR changes, the rationale behind codes,
and how it affects the users and other developers.
-->

### This PR resolves [#2803](#2803) issue.

**Changes:**
- Displays the resources allocated to the session. Currently, the compute session node does not include a live stat field, which shows the live stat information of the main kernel associated with the session.
- Since each device has different usage, I've shown the usage below the progress bar to bring consistency to the design.

**How to test:**
- Access the Detail page of the endpoint that is currently in service.
- Verify that resource usage looks normal.

|normal|small screen|
|---|---|
|![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/lSyr8xXz1wdXALkJKzVx/b491c912-330f-4467-b995-b0d05aded2ea.png)|![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/lSyr8xXz1wdXALkJKzVx/c1bb67ea-d3c5-4b79-8ee1-18fb9e052906.png)|

**Checklist:** (if applicable)

- [x] Mention to the original issue
- [ ] Documentation
- [ ] Minium required manager version
- [ ] Specific setting for review (eg., KB link, endpoint or how to setup)
- [ ] Minimum requirements to check during review
- [ ] Test case(s) to demonstrate the difference of before/after
@ironAiken2 ironAiken2 force-pushed the feat/session-usage-monitor branch from f84a448 to 0900b2e Compare January 2, 2025 04:24
@graphite-app graphite-app bot merged commit 0900b2e into main Jan 2, 2025
6 checks passed
@graphite-app graphite-app bot deleted the feat/session-usage-monitor branch January 2, 2025 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session usage component in React
3 participants